Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add type declarations where backwards-compatible #8544

Merged
merged 1 commit into from
Apr 19, 2021

Conversation

greg0ire
Copy link
Member

@greg0ire greg0ire commented Mar 14, 2021

This includes:

  • private methods
  • return type declarations of final protected methods
  • return type declarations of public and protected methods of final classes

Parameter type declarations are a more delicate matter and should
probably be handled separately to make it easier to catch issues during
code review.

Type declarations can be more trusted than simple phpdoc when it
comes to static analysis, having them means we can infer the phpdoc of
calling methods with confidence.

Note that it seems that some of the phpdoc I initially inferred these
declarations from were apparently wrong, in particular some mentioning
Doctrine\Dbal\Statement when was is really passed around is
Doctrine\Dbal\Driver\Statement.

Closes #8538

@greg0ire greg0ire requested a review from beberlei March 15, 2021 18:40
@greg0ire greg0ire linked an issue Mar 15, 2021 that may be closed by this pull request
@greg0ire
Copy link
Member Author

Thanks for the careful review @simonberger , much appreciated 👍

simonberger
simonberger previously approved these changes Mar 16, 2021
lib/Doctrine/ORM/Mapping/Driver/XmlDriver.php Show resolved Hide resolved
lib/Doctrine/ORM/Tools/EntityRepositoryGenerator.php Outdated Show resolved Hide resolved
{
$namespace = $this->getClassNamespace($fullClassName);

return $namespace ? 'namespace ' . $namespace . ';' : '';
}

/**
* @param string $fullClassName
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

class-string

lib/Doctrine/ORM/Tools/ResolveTargetEntityListener.php Outdated Show resolved Hide resolved
@greg0ire greg0ire force-pushed the bc-type-declarations branch 5 times, most recently from 5d090db to 507a115 Compare April 13, 2021 19:34
Copy link
Member

@ostrolucky ostrolucky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One comment remaining. Also, you are adding object type declarations, but these are being removed in #8613. That will cause mess of a conflict.

use Doctrine\DBAL\LockMode;
use Doctrine\DBAL\Platforms\AbstractPlatform;
use Doctrine\DBAL\Statement;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unsure if we should wait for doctrine/dbal#4596 and revert this change in use statements. I think that change might be good because more forward compatible.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll leave you decide. But to me, such change seems a bit unrelated to this PR

@greg0ire
Copy link
Member Author

I will wait until #8613 is resolved

@greg0ire greg0ire force-pushed the bc-type-declarations branch 4 times, most recently from 105670f to 6afe86a Compare April 18, 2021 17:38
@beberlei beberlei added this to the 2.9.0 milestone Apr 18, 2021
ostrolucky
ostrolucky previously approved these changes Apr 19, 2021
@greg0ire
Copy link
Member Author

The 7.1 PR has been merged, let's rebase to be 100% sure it still works

This includes:
- private methods
- return type declarations of final protected methods
- return type declarations of public and protected methods of final classes

Parameter type declarations are a more delicate matter and should
probably be handled separately to make it easier to catch issues during
code review.

Type declarations can be more trusted than simple phpdoc when it
comes to static analysis, having them means we can infer the phpdoc of
calling methods with confidence.

Note that it seems that some of the phpdoc I initially inferred these
declarations from were apparently wrong, in particular some mentioning
Doctrine\Dbal\Statement when was is really passed around is
Doctrine\Dbal\Driver\Statement.
@greg0ire greg0ire merged commit 72f5003 into doctrine:2.9.x Apr 19, 2021
@greg0ire greg0ire deleted the bc-type-declarations branch April 19, 2021 12:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add backwards-compatible type declarations
5 participants